Skip to content

Conversation

@tpetazzoni
Copy link

In some cases, even if Swig is found and Python3 is found, it may not be desirable to build Python support in avrdude, so this commit adds an ENABLE_PYTHON_SUPPORT option to be able to explicitly disable using Python support (unfortunately CMake doesn't allow passing arguments that would prevent it from finding Swig/Python 3 if available).

To preserve existing behavior, this option defaults to enabled (ON).

In some cases, even if Swig is found and Python3 is found, it may not
be desirable to build Python support in avrdude, so this commit adds
an ENABLE_PYTHON_SUPPORT option to be able to explicitly disable using
Python support (unfortunately CMake doesn't allow passing arguments
that would prevent it from finding Swig/Python 3 if available).

To preserve existing behavior, this option defaults to enabled (ON).

Signed-off-by: Thomas Petazzoni <[email protected]>
@mcuee
Copy link
Collaborator

mcuee commented Sep 1, 2025

Looks like a good improvement to me.

@mcuee
Copy link
Collaborator

mcuee commented Sep 1, 2025

@ndim annd @Youw

Just wondering if you got the time to take a look at this PR to see if it is good to go. Thanks.

@Youw
Copy link
Contributor

Youw commented Sep 1, 2025

White technically the implementation is correct, and preserves backward compatibility, it actually introduces a new confusing workflow which I've seen in similar context but in other project:
One may want to try to use the new option as:

cmake -B avrdude -DENABLE_PYTHON_SUPPORT=True ...

And the CMake configuration will finish w/o errors, but in case if python/swig is missing, the python support will be disabled, and that part is a bit confusing considering the variable name.

Alternatives:

  • make it a FATAL_ERROR in case if ENABLE_PYTHON_SUPPORT=True but python/swig is not found (breaks backward compatibility);
  • instead of making ENABLE_PYTHON_SUPPORT a proper cmake option, simply check if(DEFINED ENABLE_PYTHON_SUPPORT), and only then have a FATAL_ERROR in case if ENABLE_PYTHON_SUPPORT=True but python/swig is not found; this way we have backward compatibility (with ENABLE_PYTHON_SUPPORT not defined - fully preserve old behavior), but it complicates implementation a bit;
  • introduce a different name for the variable, like FORCE_DISABLE_PYTHON_SUPPORT (and invert current logic); this would preserve backward compatibility same way it is now, and will be less confusing when used by users who don't have the historical context;

@stefanrueger
Copy link
Collaborator

@Youw Thanks for your considered review!

@tpetazzoni I personally would prefer either one of the two

  • Check if(DEFINED ENABLE_PYTHON_SUPPORT) ...
  • Introduce a different name for the variable, like FORCE_DISABLE_PYTHON_SUPPORT (and invert current logic)

Also, the is an unsuccessful check, which if caused by this PR would need addressing before merging.

@mcuee mcuee added the build_CI Related to CMake, auto-tools build infrastructure and github CI label Sep 13, 2025
@mcuee
Copy link
Collaborator

mcuee commented Sep 15, 2025

Hmm, the macOS build failure does not seem to be related to this PR, but due to Homebrew cmake formula conflict.

@mcuee
Copy link
Collaborator

mcuee commented Sep 15, 2025

Hmm, the macOS build failure does not seem to be related to this PR, but due to Homebrew cmake formula conflict.

Looks like a transient issue, I just trigger the rebuild and everything is okay now.

The warnings are okay, as we do not know if the runner will change in the future.

Run brew install cmake flex bison swig libelf libusb libusb-compat hidapi libftdi readline libserialport
Warning: cmake 4.1.1 is already installed and up-to-date.
To reinstall 4.1.1, run:
  brew reinstall cmake

Warning: libusb 1.0.29 is already installed and up-to-date.
To reinstall 1.0.29, run:
  brew reinstall libusb

Warning: readline 8.3.1 is already installed and up-to-date.
To reinstall 8.3.1, run:
  brew reinstall readline

@mcuee
Copy link
Collaborator

mcuee commented Oct 21, 2025

@tpetazzoni I personally would prefer either one of the two

Check if(DEFINED ENABLE_PYTHON_SUPPORT) ...
Introduce a different name for the variable, like FORCE_DISABLE_PYTHON_SUPPORT (and invert current logic)

@tpetazzoni

Would you please address the comment from Stefan? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build_CI Related to CMake, auto-tools build infrastructure and github CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants